Anna and Ida - Slack CLI #9

idago123 · 2020-10-09T17:55:47Z

Assignment Submission: Slack CLI

Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.

Reflection

Question	Answer
How did you go about exploring the Slack API? Did you learn anything that would be useful for your next project involving an API?	We explored the api docs and learned more about what the arguments and permissions meant. We then tested the bots in the new workspace we created. After learning more about the docs we understood how to interact with Postman.
Give a short summary of the request/response cycle. Where does your program fit into that scheme?	Client makes a request and server makes a response. Our program is the client making a request to the Slack API.
How does your program check for and handle errors when using the Slack API?	unless response.code == 200 && response.parsed_response["ok"]

  raise SlackApiError, "Error when posting #{message} to #{@slack_id}, error: #{response.parsed_response["error"]}"
end

How did the design and organization of your project change over time? | We started in workspace.rb and then we redesigned our program to include other classes.
Did you use any of the inheritance idioms we've talked about in class? How? | We used an abstract and parent class (Recipient) and the user and channel classes inherited from it.
How does VCR aid in testing a program that uses an API? | Records the API responses so that you don't have to keep calling it.

tildeee

Slack CLI

Major Learning Goals/Code Review

Criteria	yes/no, and optionally any details/lines of code to reference
Practices best practices working with APIs. The `.env` is not checked into git, and no API token was directly used in the Ruby code without `ENV`.	✔️
Practices error handling with APIs. For all pieces of code that make an API call, it handles API requests that come back with errors/error status codes appropriately.	Mostly -- It might be good to check in `Channel#list_all` and `User#list_all` to have behavior in case the `self.get` returns unsuccessfully. Your code actually does break if `self.get` returned unsuccessfully! It would also be nice to start anticipating failures like this in the tests for those methods, too!
Implements inheritance and inheritance idioms. There is a `Recipient` class. `User` and `Channel` inherit from `Recipient`. In `Recipient`, there are appropriate methods defined that are used in both `User` and `Channel`. Some may be implemented. Some may be template methods.	✔️
Practices clean code. `lib/slack.rb` only interacts with `Workspace` to show a separation of responsibilities. Complex logic is broken into smaller helper methods.	✔️
Practices instance methods vs. class methods appropriately. The methods to list all Channels or Users is likely a class method within those respective classes.	✔️
Practices best practices for testing. The project has and uses VCR mocking when running tests, and can run offline.	✔️
Practices writing tests. The `User`, `Channel`, and `Workspace` classes have unit tests.	✔️
Practices writing tests. There are tests for sending messages (the location of these tests may differ, but is likely in `Recipient`)	✔️
Practices git with at least 15 small commits and meaningful commit messages	There weren't that many commits, but more importantly, commit messages should describe the changes in the commit. They should not be something like "Wave 2 complete," or "changed tests." Try things like, "implements the list_all method in Channel" or "updates the Workspace tests to use helper methods," or "refactors the main script to use a case statement"

Functional Requirements

Functional Requirement	yes/no
As a user of the CLI program, I can list users and channels	✔️
As a user of the CLI program, I can select users and channels	✔️
As a user of the CLI program, I can show the details of a selected user or channel	✔️
As a user of the CLI program, when I input something inappropriately, the program runs without crashing	✔️

Overall Feedback

Overall Feedback	Criteria	yes/no
Green (Meets/Exceeds Standards)	7+ in Code Review && 3+ in Functional Requirements	✔️
Yellow (Approaches Standards)	6+ in Code Review && 2+ in Functional Requirements
Red (Not at Standard)	0-5 in Code Review or 0,1 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging

Additional Feedback

Great work on this project, Anna and Ida!

Overall, your code was great! The object-oriented design of the project (the classes, inheritance, methods, etc) is very similar to what I expected and hoped for, and your code and comprehension question answers show understanding here.

Also, you all wrote the tests very well, using VCR very well.

I only have two pieces of feedback to emphasize:

I would make sure to continue anticipating failure cases ("edge cases") in code and tests. Y'all did this well for the send_message method, but not quite for the list_all methods
The send_message test should either be in both channel_test AND user_test, or only in a new file recipient_test. I emphasize this to help build consistency for yourselves and your future teammates

I also am adding some smaller thoughts about code style and refactoring. There's a big one about constants/your API tokens in ENV, so let me know if any questions come up!

Great work on the project overall! Keep it up!

Code Style Bonus Awards

Was the code particularly impressive in code style for any of these reasons (or more...?)

Quality	Yes?
Perfect Indentation	✅
Elegant/Clever
Descriptive/Readable
Concise
Logical/Organized

tildeee · 2020-10-23T05:27:17Z

lib/channel.rb

+    response = self.get(CHANNELS_URL, PARAMS)
+    response["channels"].map do |channel|
+      Channel.new(
+          slack_id: channel["id"],
+          name: channel["name"],
+          topic: channel["topic"]["value"],
+          member_count: channel["num_members"]
+          )
+    end


I mentioned this in the feedback table, but just to repeat: If response on lin 17 doesn't come back with a success of 200, then on line 18, response["channels"] will likely be nil, and response["channels"].map will say "There is no method map for nil", and break!

You all had a good pattern in the send_message method, where you checked the response code and raised an error, and also tested for that case.

tildeee · 2020-10-23T05:27:44Z

lib/recipient.rb

+  USERS_URL = 'https://slack.com/api/users.list'
+  CHANNELS_URL = 'https://slack.com/api/conversations.list'
+  MESSAGE_URL = 'https://slack.com/api/chat.postMessage'
+  PARAMS = 'SLACK_API_TOKEN'
+  SLACK_BOT_TOKEN = 'SLACK_BOT_API_TOKEN'


Smart for making these into constants!

tildeee · 2020-10-23T05:32:14Z

lib/recipient.rb

+  PARAMS = 'SLACK_API_TOKEN'
+  SLACK_BOT_TOKEN = 'SLACK_BOT_API_TOKEN'


These constants end up being kind of confusing throughout your project.

What does the name PARAMS mean? What are these two constants doing? This project doesn't need two API tokens to be used. In your code, the send_message method directly references ENV[SLACK_BOT_TOKEN], which doesn't use either of these constants, parts of your code pass around PARAMS, and your code actually never uses the constant SLACK_BOT_TOKEN

I'm wondering if there's some confusion about constants, and why I might be confused by your use of constants. If that's the case, let me know and I can go over this stuff!

If this comment makes sense to y'all and you see how it feels messy, then I'd encourage y'all to see code like this in the future and refactor and simplify! It might have been more direct if you all only used one API token in this project consistently, and using one syntax consistently.

tildeee · 2020-10-23T05:33:35Z

test/channel_test.rb

+  describe "send_message" do
+    it "recipient" do
+      VCR.use_cassette("channel") do
+        expect {
+          new_recipient = Channel.new(slack_id: "fake", name: "fake", topic: "fake", member_count: "fake")
+          new_recipient.send_message("This post should not work")
+        }.must_raise SlackApiError
+      end
+    end
+    it "sends a message to a channel" do
+      VCR.use_cassette("channel") do
+          new_recipient = Channel.new(slack_id: "C01BW93UN8N", name: "random", topic: "super random!", member_count: 3)
+          expect(new_recipient.send_message("hello testing").code).must_equal 200
+      end
+    end
+
+  end


The tests for the send_message method are great! Great work!

Technically, since the send_message method is defined in the Recipient class. However, you only test it here, in the Channel class.

In my opinion, I think it'd be better to do one of these two options:

Just put the send_message test in a new test file, recipient_test.rb

Or have a test for send_message in Channel AND one in User

This could feel like a lot of effort for not a lot, but remember:

Small, numerous, organized files are encouraged in programming!

Imagine a new team member who needs to find the tests for Recipient.send_message. If we don't have a consistent organization for where the tests for send_message are, then a dev might think the tests don't exist, and rewrite them. Then it'll just be really sad that two developers spent time writing duplicate tests for the same thing.

Or consider this case. Six months from now, we get a new feature, and we need to update the Recipient.send_message method. But when we update it, all of a sudden, tests in the channel_test.rb file fail, but nothing else! What?! That would be very surprising to the devs at that moment.

tildeee · 2020-10-23T05:34:41Z

test/user_test.rb

+
+describe "User class" do
+
+  describe "list_all" do


As mentioned in a few other places, it would be great if in the future you all also had a test for User.list_all that anticipated the API call failing!

tildeee · 2020-10-23T05:48:37Z

test/channel_test.rb

+  end
+
+  describe "send_message" do
+    it "recipient" do


Very small nitpick: this it block's description isn't very meaningful to me ("recipient") and it would be helpful to have a better name here.

idago123 added 9 commits October 6, 2020 11:30

updated slack

221ac6a

wave 1

32143c2

wave 2, testing

cabd626

updated tests with vcr

2a94ff7

updated channel test, user test, workspace test

99d6a33

wave 3 and added details test

ae12992

updated user details

c164243

added new tests

1807024

deleted file

0b890a1

tildeee reviewed Oct 23, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Anna and Ida - Slack CLI #9

Anna and Ida - Slack CLI #9

idago123 commented Oct 9, 2020

tildeee left a comment •

edited

Loading

tildeee Oct 23, 2020

tildeee Oct 23, 2020

tildeee Oct 23, 2020

tildeee Oct 23, 2020

tildeee Oct 23, 2020

tildeee Oct 23, 2020

		PARAMS = 'SLACK_API_TOKEN'
		SLACK_BOT_TOKEN = 'SLACK_BOT_API_TOKEN'

Anna and Ida - Slack CLI #9

Are you sure you want to change the base?

Anna and Ida - Slack CLI #9

Conversation

idago123 commented Oct 9, 2020

Assignment Submission: Slack CLI

Reflection

tildeee left a comment • edited Loading

Choose a reason for hiding this comment

Slack CLI

Major Learning Goals/Code Review

Functional Requirements

Overall Feedback

Additional Feedback

Code Style Bonus Awards

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee Oct 23, 2020

Choose a reason for hiding this comment

tildeee left a comment •

edited

Loading